Bugfix- wolf ssl add0 chain cert not incrementing certchain and causing TLS1.3 to fail#10517
Conversation
|
@julek-wolfssl as we discussed on the ticket this is the PR for the bug in add0 function + added a test for it |
|
Can one of the admins verify this patch? |
…n SendTls13Certificate not updating the chain count made TLS 1.3 handshake fail and leave with leaft-only certficiate and then peer rejected the chain
56b78bc to
416d317
Compare
|
@dgarske I fixed the whitespace issue + rebased to master for the second test . can we re-run workflow here? |
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Low] Test verifies the counter but not the end-to-end chain-send behavior it fixes —
tests/api.c:3696-3739 - [Info] Increment is unconditional while CTX equivalent guards it with WOLFSSL_TLS13 —
src/ssl_load.c:5225
Review generated by Skoll
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| static int test_wolfSSL_add0_chain_cert_increments_count(void) |
There was a problem hiding this comment.
🔵 [Low] Test verifies the counter but not the end-to-end chain-send behavior it fixes
The new test directly asserts ssl->buffers.certChainCnt increments per chain cert, which is a good, targeted regression guard for this specific fix. It does not, however, exercise the actual symptom (a TLS 1.3 handshake emitting a full chain rather than a leaf-only Certificate message), nor the wolfSSL_add1_chain_cert path which funnels through wolfSSL_add0_chain_cert and is therefore also affected by the fix. The unit test is acceptable as-is given how much harder a full handshake assertion would be, but coverage of the add1 wrapper would be cheap and worthwhile. Also note: if SSL_add0_chain_cert ever returned 0 inside the loop, the just-loaded x509 would leak since it is unconditionally set to NULL afterward without being freed on the failure path -- only a concern under a future regression, not current behavior.
Fix: Consider adding a parallel assertion via SSL_add1_chain_cert (verifying both the count increments and the X509 ref count is bumped), and/or a comment noting the unit test stands in for the harder-to-author TLS 1.3 full-chain handshake check.
| ssl->buffers.certChainCnt++; | ||
| /* We now own cert chain. */ | ||
| ssl->buffers.weOwnCertChain = 1; | ||
| /* Create a stack to put certificate into. */ |
There was a problem hiding this comment.
⚪ [Info] Increment is unconditional while CTX equivalent guards it with WOLFSSL_TLS13
The new ssl->buffers.certChainCnt++; is unconditional, whereas the analogous CTX helper wolfssl_ctx_add_to_chain (src/ssl_load.c:4968-4971) wraps ctx->certChainCnt++; in #ifdef WOLFSSL_TLS13. This is a minor stylistic inconsistency, not a bug: the certChainCnt member is always defined (under #ifndef NO_CERTS, wolfssl/internal.h:5029), so the unconditional increment always compiles, and the count is consumed both by TLS 1.3 (SendTls13Certificate) and by the non-TLS1.3 OCSP-multi path (MIN(cnt, certChainCnt + 1) at src/internal.c:26269). The unconditional form here is arguably more correct than the CTX version because it keeps the count accurate even in non-TLS1.3 builds. No change required; flagging only for awareness of the divergence between the two helpers.
Fix: Leave as-is (unconditional is fine and slightly more correct). Optionally note the divergence, or align the CTX helper to also increment unconditionally in a follow-up for consistency.
Description
in TLS 1.3 handshake we're dependent on the cert chain cnt in SendTls13Certificate . not updating the chain count upon using
SSL_add0_chain_certmade TLS 1.3 to leave with leaf-only certificate and then peer rejected the chain and handshake failed.Testing
Tested it on my platform after encountering the handshake bug as part of migrating from openssl -> wolfssl + made a simple unitest for it
Checklist